Skip to content

Revert "PILOT-7878: CLI download of large folder on bd2data resulting in global.unhandled_exception error (Internal Server error)"#239

Merged
vadimsoltan merged 1 commit into
developfrom
revert-238-PILOT-7878
Jun 11, 2025
Merged

Revert "PILOT-7878: CLI download of large folder on bd2data resulting in global.unhandled_exception error (Internal Server error)"#239
vadimsoltan merged 1 commit into
developfrom
revert-238-PILOT-7878

Conversation

@vadimsoltan
Copy link
Copy Markdown
Contributor

@vadimsoltan vadimsoltan commented Jun 11, 2025

Reverts #238

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 11, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
__init__.py00100% 
pilotcli.py0620%5, 7–10, 12–19, 22–23, 25–26, 28–29, 31–35, 37–40, 42, 44–45, 47–48, 51, 54–60, 62, 64–67, 69–74, 77–83, 86–88
commands
   __init__.py00100% 
   container_registry.py01959%16, 22–24, 31–33, 41–44, 50–53, 62–65
   dataset.py0989%20, 38–39, 47, 82–83, 92, 126, 130
   entry_point.py01185%41, 47, 75, 98–100, 102–106
   file.py03886%48, 159–162, 166, 171, 174–176, 307–314, 316, 318, 332–338, 340, 369, 371, 375, 421, 424, 439–441, 447–448
   folder.py0196%23
   project.py0295%18, 61
   user.py0983%28, 50–51, 59–60, 68–69, 90–91
configs
   __init__.py00100% 
   app_config.py00100% 
   config.py00100% 
   user_config.py02382%35, 75, 84, 95, 128, 137–138, 141, 151, 157, 161, 165, 169, 173–174, 178, 182–183, 187, 191–192, 200, 204
   utils.py03536%13–14, 16–19, 22–23, 25, 27, 31–32, 34, 36–37, 40, 43, 45–47, 49–51, 54–57, 59, 67, 69–72, 74–75
models
   __init__.py00100% 
   enums.py00100% 
   item.py00100% 
   service_meta_class.py0180%9
   singleton.py00100% 
   upload_form.py0433%28, 40–42
resources
   custom_error.py00100% 
   custom_help.py00100% 
services
   __init__.py00100% 
services/clients
   __init__.py00100% 
   base_auth_client.py0196%54
   base_client.py0295%53, 104
services/container_registry_manager
   container_registry_manager.py010817%19–20, 23–26, 29–33, 36–44, 48–60, 62–64, 68–82, 84–86, 90–99, 101–103, 107–110, 125–136, 140–143, 147–162, 164, 166–169
services/crypto
   __init__.py00100% 
   crypto.py01940%35, 43–47, 49, 59–61, 69–75, 77–78
services/dataset_manager
   dataset_detail.py02271%38, 59, 66–71, 73–75, 77–86, 88
   dataset_download.py02282%53, 63–65, 74–76, 92, 94–95, 97–98, 100, 102–104, 112–115, 149, 164
   dataset_list.py0881%36–41, 43, 52
   model.py00100% 
services/file_manager
   __init__.py00100% 
   file_list.py01285%60–66, 86–88, 100, 102
   file_manifests.py02578%70, 181–195, 197, 202–206, 208–210
   file_tag.py04031%24–25, 29–33, 35, 38–50, 52–54, 56–57, 61–63, 66–70, 72–75, 77–78
services/file_manager/file_download
   __init__.py00100% 
   download_client.py012754%53–62, 80–81, 123–127, 129–131, 143–144, 149–150, 154–157, 159–162, 164–169, 172, 174–175, 177–179, 182, 184, 187, 191, 194–195, 197, 201, 215–217, 223–224, 233–234, 236–238, 254–255, 286, 291, 297–307, 309–310, 313–314, 319–320, 324–329, 333–334, 337–339, 341–352, 359, 364, 379, 383–385, 387–400, 402
   model.py00100% 
services/file_manager/file_metadata
   __init__.py00100% 
   file_metadata_client.py0395%98–100
   folder_client.py0491%50, 79–81
services/file_manager/file_move
   __init__.py00100% 
   file_move_client.py03759%60–64, 66, 74, 79–83, 86–94, 96, 98–103, 114, 116–120, 122, 172–173
services/file_manager/file_trash
   __init__.py00100% 
   file_trash_client.py0296%69, 90
   utils.py0392%39, 63, 70
services/file_manager/file_upload
   __init__.py00100% 
   exception.py00100% 
   file_upload.py01791%104–106, 118, 143, 168–170, 199–200, 209, 410–412, 421, 424, 428
   models.py0296%24, 44
   upload_client.py04977%106–109, 149, 223, 236–239, 241–254, 256, 258–262, 264–268, 270–271, 372–373, 375, 445, 451–452, 457–460, 462–463
   upload_validator.py01373%54–58, 60, 64–66, 69–70, 75, 77
services/logger_services
   __init__.py00100% 
   debugging_log.py00100% 
   log_functions.py00100% 
services/output_manager
   __init__.py00100% 
   error_handler.py00100% 
   help_page.py0791%13–17, 19, 21
   message_handler.py05474%24, 46, 51, 66–68, 73, 101, 106, 113, 118, 123, 127, 143, 178, 188, 197, 215, 237, 281–292, 303–308, 310–316, 327, 331–332, 336–337, 339–340, 344, 348, 352
services/project_manager
   __init__.py00100% 
   project.py0684%40, 46–47, 49–51
services/user_authentication
   __init__.py00100% 
   decorator.py0293%27, 30
   token_manager.py0888%42–46, 73–74, 91
   user_login_logout.py01684%29–30, 41, 126, 130–136, 138–140, 144–145
utils
   __init__.py00100% 
   aggregated.py02884%56, 66–68, 82–84, 116, 130–131, 144, 168–175, 177–178, 206, 214, 231–232, 271–272, 299
TOTAL375485177% 

@vadimsoltan vadimsoltan requested a review from Copilot June 11, 2025 19:57
@vadimsoltan vadimsoltan merged commit bf46458 into develop Jun 11, 2025
2 checks passed
@vadimsoltan vadimsoltan deleted the revert-238-PILOT-7878 branch June 11, 2025 19:59
Copilot AI reviewed Jun 11, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR reverts the previous changes for CLI file download error handling, restoring the original download logic and versioning.

  • Removed timeout/retry logic and its associated enum and test.
  • Reverted package version and documentation entries back from 3.19.5 to 3.19.4.
  • Simplified the download_file method by removing explicit timeout config and moving the return outside the exception block.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/app/services/file_manager/file_download/test_file_download_client.py Deleted the test for read-timeout retry behavior
app/services/file_manager/file_download/download_client.py Removed retry loop, timeout parameters, and adjusted return logic
app/services/output_manager/error_handler.py Removed DOWNLOAD_TIMEOUT enum constant
pyproject.toml Reverted version from 3.19.5 to 3.19.4
docs/compatible_version.ndjson Removed entry for CLI version 3.19.5
Comments suppressed due to low confidence (1)

tests/app/services/file_manager/file_download/test_file_download_client.py:318

  • The removal of this test eliminates coverage for handling download exceptions. Add new tests to verify how download_file behaves when errors occur under the updated logic.
def test_download_file_read_timeout_retry_and_failure(mocker):

Comment thread app/services/file_manager/file_download/download_client.py
logger.info('Download complete')
except Exception as e:
logger.error(f'Error downloading: {e}')
return local_filename
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function unconditionally returns the filename even if an exception occurred. Consider returning None or re-raising the exception to signal a failed download.

Suggested change
return local_filename
return None

Copilot uses AI. Check for mistakes.
Comment thread app/services/file_manager/file_download/download_client.py
logger.error(f'Error downloading: {e}')
return None
try:
with httpx.stream('GET', url) as r:
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the timeout parameters may allow the stream to hang indefinitely. Reintroduce appropriate timeout settings to guard against stalled connections.

Suggested change
with httpx.stream('GET', url) as r:
with httpx.stream('GET', url, timeout=10) as r: # 10-second timeout

Copilot uses AI. Check for mistakes.
Comment thread app/services/file_manager/file_download/download_client.py

@require_valid_token()
def download_file(self, url, local_filename, download_mode='single'): # noqa: C901
def download_file(self, url, local_filename, download_mode='single'):
Copy link

Copilot AI Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The complexity suppression (# noqa: C901) was removed but the method remains large. Consider refactoring into smaller helper functions for clarity and maintainability.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Samantha77 Samantha77 Samantha77 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vadimsoltan @Samantha77